Optimize /ui/dags endpoint serialization#61483
Optimize /ui/dags endpoint serialization#61483john-rodriguez-mgni wants to merge 1 commit intoapache:mainfrom
Conversation
|
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide (https://github.com/apache/airflow/blob/main/contributing-docs/README.rst)
|
airflow-core/src/airflow/api_fastapi/core_api/routes/ui/dags.py
Outdated
Show resolved
Hide resolved
b8ac2cf to
1350bfa
Compare
pierrejeambrun
left a comment
There was a problem hiding this comment.
Nice, thanks for the PR.
A few questions/suggestions, but overall looking good.
airflow-core/src/airflow/api_fastapi/core_api/routes/ui/dags.py
Outdated
Show resolved
Hide resolved
pierrejeambrun
left a comment
There was a problem hiding this comment.
Nice thank for the PR.
A couple of suggestion / improvements before we can merge I think.
1350bfa to
f1a3a5c
Compare
airflow-core/src/airflow/api_fastapi/core_api/datamodels/dags.py
Outdated
Show resolved
Hide resolved
| DagRun.duration.expression, # type: ignore[attr-defined] | ||
| DagRun.duration, |
There was a problem hiding this comment.
It seems we have to keep DagRun.duration.expression, # type: ignore[attr-defined] for mypy.
pierrejeambrun
left a comment
There was a problem hiding this comment.
LGTM, we can merge after we addressed above comments.
f1a3a5c to
153d4c9
Compare
jason810496
left a comment
There was a problem hiding this comment.
Nice! Thanks for the improvement.
airflow-core/src/airflow/api_fastapi/core_api/routes/ui/dags.py
Outdated
Show resolved
Hide resolved
| DagRun.duration.expression, # type: ignore[attr-defined] | ||
| DagRun.duration, |
There was a problem hiding this comment.
It seems we have to keep DagRun.duration.expression, # type: ignore[attr-defined] for mypy.
|
Just an update on this... since opening this PR, I have included this patch in our dev and QA environments and validated there are no issues and that it resolves the load time for the ui/dags endpoint seen with the base airflow 3.1.7 release sans patches. Earlier today I released it to our production environment and are not seeing any issues either and can confirm that load time for the ui/dags endpoint is fixed. Let me know what else is required to move this along. Again, really appreciate your feedback and thank you for your help throughout this process! |
pierrejeambrun
left a comment
There was a problem hiding this comment.
Let me know what else is required to move this along. Again, really appreciate your feedback and thank you for your help throughout this process!
Can you just fix the CI please (mypy static check error). There are two discussion above that explains what the problem is.
Basically we need to keep the # type: ignore[attr-defined] and revert the change to the get_latest_run_info which is creating mypy errors and isn't really needed for that PR. (that endpoint isn't the performance bottleneck)
153d4c9 to
86dd884
Compare
Made updates as requested! |
|
@pierrejeambrun just checking on this to see if there was anything else required on my end, I have made the changes necessary to address the CI failures... I think we just need someone to approve the workflows. |
pierrejeambrun
left a comment
There was a problem hiding this comment.
Thanks, CI is running it should be better 🤞
86dd884 to
460629c
Compare
pierrejeambrun
left a comment
There was a problem hiding this comment.
CI need fixing some tests are failing, can you take a look please.
|
@pierrejeambrun it seems that all 10 CI failures failures trace back to the same root cause; the The immediate fix is simple — add Thoughts? |
|
@pierrejeambrun any thoughts on the proposed implementation: #61483 (comment) |
This PR addresses a significant performance issue in the /ui/dags endpoint
where page load times scaled poorly with the number of DAGs (12-16 seconds
for just 25 DAGs in our testing).
Two optimizations are implemented:
1. Cache URLSafeSerializer for file_token generation
- Previously, a new URLSafeSerializer was instantiated and
conf.get_mandatory_value() was called for every DAG
- Now uses @lru_cache to create the serializer once and reuse it
2. Eliminate redundant Pydantic validation in response construction
- The original pattern used model_validate -> model_dump -> model_validate
which caused triple serialization overhead per DAG
- Now validates once with DAGResponse.model_validate(), then uses
model_construct() to build DAGWithLatestDagRunsResponse
Together, these changes reduced page load time from 12-16 seconds to
~130ms in our dev environment.
Co-authored-by: Cursor <cursoragent@cursor.com>
460629c to
fa124e2
Compare
|
@pierrejeambrun I have decided to build the dict dynamically from DAGResponse.model_fields to make this robust. I ran the CI test locally and confirmed that fixes the issue. I also deployed this locally to our Airflow dev instances and verified that the dags endpoint performance gain is still present. We are ready to approve the CI workflows again. Please approve them at your earliest convenience. |
Summary
This PR addresses a significant performance issue in the
/ui/dagsendpoint where page load times scaled poorly with the number of DAGs (12-16 seconds for just 25 DAGs in our testing).Two optimizations are implemented:
1. Cache URLSafeSerializer for file_token generation
Previously, a new
URLSafeSerializerwas instantiated andconf.get_mandatory_value()was called for every DAG in the response. Now uses@lru_cacheto create the serializer once and reuse it.2. Eliminate redundant Pydantic validation in response construction
The original pattern used:
This caused triple serialization overhead per DAG (validate → dump → validate). The fix validates once with
DAGResponse.model_validate(), then usesmodel_construct()to buildDAGWithLatestDagRunsResponsewithout redundant validation.Results
Together, these changes reduced page load time from 12-16 seconds to ~130ms in our dev environment.
Test Plan
Made with Cursor